fix(native): run shutdown in thread in uwsgi#19086
Conversation
Codeowners resolved as |
|
432cbd8 to
da2b375
Compare
BenchmarksBenchmark execution time: 2026-07-17 17:37:02 Comparing candidate commit 8025fa7 in PR branch Found 0 performance improvements and 8 performance regressions! Performance is the same for 607 metrics, 10 unstable metrics. scenario:iast_aspects-re_search_aspect
scenario:iastaspects-add_aspect
scenario:iastaspects-casefold_noaspect
scenario:iastaspects-strip_aspect
scenario:iastaspectsospath-ospathbasename_aspect
scenario:iastaspectssplit-rsplit_aspect
scenario:span-start
scenario:telemetryaddmetric-1-count-metric-1-times
|
da2b375 to
321c8c0
Compare
321c8c0 to
f017a82
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12f5d61945
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| already been stopped (e.g. via TraceExporter.shutdown). | ||
| """ | ||
| super().shutdown(timeout_ms=timeout_ms) | ||
| using_uwsgi = importlib.util.find_spec("uwsgi") is not None |
There was a problem hiding this comment.
When running under uWSGI on Python versions where the server injects the synthetic uwsgi module without a module spec, importlib.util.find_spec("uwsgi") raises ValueError instead of returning a truthy value. Since _atexit() catches that exception, SIGTERM exits skip shutdown_in_thread() entirely, so the uWSGI crash path this change is meant to fix still uses no native-runtime shutdown; use import uwsgi/sys.modules or the existing uWSGI helper and handle ValueError here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
👍🏻 I agree with this feedback, I thik uwsgi directly injects the module in sys.modules, so doing a pyimport of uwsgi or checking sys.modules["uwsgi"] would be better
| self._test_session_token = token | ||
| old_exporter = self._exporter | ||
| self._exporter = self._create_exporter() | ||
| old_exporter.shutdown(3_000_000_000) |
There was a problem hiding this comment.
This new explicit shutdown covers token/API swaps, but NativeWriter.recreate() still catches ServiceStatusError and returns a new writer without stopping the old exporter when the writer was never started, which happens in fork-child recreation and LLMObs/AppSec reconfiguration before the first trace. Since this commit removed the Rust Drop shutdown fallback, those old exporters leave their SharedRuntime workers registered/running until process exit, causing duplicate native workers after each such recreate; add the same exporter shutdown/drop cleanup on the stopped-writer recreate path.
Useful? React with 👍 / 👎.
brettlangdon
left a comment
brettlangdon
left a comment
There was a problem hiding this comment.
this seems ok, do we have a regression test we can add?
| already been stopped (e.g. via TraceExporter.shutdown). | ||
| """ | ||
| super().shutdown(timeout_ms=timeout_ms) | ||
| using_uwsgi = importlib.util.find_spec("uwsgi") is not None |
There was a problem hiding this comment.
👍🏻 I agree with this feedback, I thik uwsgi directly injects the module in sys.modules, so doing a pyimport of uwsgi or checking sys.modules["uwsgi"] would be better
2 participants
Description
Fix uwsgi crash caused by the thread local storage being destroyed when calling
shutdown.This is done by running the shared runtime shutdown in a new thread with a clean thread local storage.
This also remove the Drop implementation in the TraceExporter which called shutdown. This was used as a safeguard against dropping a trace exporter without shutting down the workers. This is replace by calling shutdown when the exporter is dropped in the writer to avoid calling it at exit.
Testing
The regression test for uwsgi crash is passing.
Risks
Additional Notes